[DYN-8894] When groups expand, auto layout relocates adjacent groups/nodes to avoid overlaps#16333
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8894
|
Thank you @ivaylo-matov, @jasonstratton is out this week so PR review could be delayed. @reddyashish @zeusongit Would you help if you have time this week? |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances group collapse behavior by allowing optional input and unconnected output ports to be hidden, adds a “collapse to minimal size” option, and implements auto-layout of adjacent elements when a group expands.
- Preferences panel UI updated with toggles for optional/unconnected ports and minimal-size collapse
- Annotation view and view-model extended for proxy port controls, port‐toggle styling, and dynamic repositioning
- Serialization, model classes, and preference settings updated to persist and respect new collapse state
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DynamoCoreWpf/Views/Menu/PreferencesView.xaml | Added rows and toggle UI for port collapse and min‐size |
| src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs | Refactored collapsed layout, removed old bindings, added port‐toggle grids |
| src/DynamoCoreWpf/ViewModels/Menu/PreferencesViewModel.cs | Bound new preference properties |
| src/DynamoCoreWpf/ViewModels/Core/Converters/SerializationConverters.cs | Wrote new collapse flags to JSON |
| src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs | Added port collections, proxy creation, layout/update logic |
| src/DynamoCoreWpf/UI/Converters.cs | Enhanced color converter for toggle styling; new angle converter |
| src/DynamoCoreCore/Graph/Annotations/AnnotationModel.cs | Introduced serialization properties, refactored boundary calculations |
| src/DynamoCore/Configuration/PreferenceSettings.cs | Default values for new collapse preferences |
Files not reviewed (1)
- src/DynamoCoreWpf/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (5)
src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs:1489
- Removing the width and height bindings on the collapsed rectangle may break size updates; ensure that MinWidthOnCollapsed and MinCollapsedPortAreaHeight drive its layout instead of direct binding.
// Set bindings
src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs:3050
- [nitpick] The same variable name
optionalStyleis reused for both input and output port controls. Rename one tounconnectedStyleto avoid confusion.
var optionalStyle = new Style(typeof(ItemsControl));
src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs:1409
- New layout‐adjustment logic in
UpdateLayoutForGroupExpansionshould be covered by unit or integration tests to verify adjacent node repositioning.
internal void UpdateLayoutForGroupExpansion()
src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs:2904
- [nitpick] Public‐facing private methods like
CreateInputPortsGridlack XML doc comments. Adding summaries will help maintainability.
private Grid CreateInputPortsGrid()
src/DynamoCore/Graph/Annotations/AnnotationModel.cs:445
- New public properties for port collapse should be documented in the API change log and considered for semantic versioning impact.
private bool isOptionalInPortsCollapsed;
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…/ivaylo-matov/Dynamo into DYN-8894-GroupAutoLayout_250624_
jasonstratton
left a comment
jasonstratton
left a comment
There was a problem hiding this comment.
I made one nitpick question and two other questions not about the code under review, but about the code pattern directed toward the Eidos team.
Since there is nothing outstanding aside from these questions and I'll approve the PR.
| goto default; | ||
| case nameof(CollapseToMinSize): | ||
| description = Res.ResourceManager.GetString(nameof(Res.PreferencesViewCollapseToMinSizeDescription), System.Globalization.CultureInfo.InvariantCulture); | ||
| goto default; |
There was a problem hiding this comment.
@reddyashish , @zeusongit , @aparajit-pratap , this is not a question about this code in particular, but I wonder about this pattern.
The comment at the top of the switch indicates:
// C# does not support going through all cases when one of the case is true
While this is true, there still will be x string comparisons, if x is the number of case statements, if e.PropertyName does not match any of the cases or matches the last case statement.
I imagine the preferences model likely does not change often and it is not a huge list of case statements, but is this pattern repeated elsewhere? Something that is run in a loop? Multiple times?
Just thinking that this string comparison pattern could lead to some performance issues.
There was a problem hiding this comment.
yes, we use this same pattern for property changes on other elements.
|
|
||
| if (pinnedNode != null && helper.ReadGuid("pinnedNode") != Guid.Empty) | ||
| pinnedNode.GUID = helper.ReadGuid("pinnedNode"); | ||
| PinnedNodeGuid = helper.ReadGuid("pinnedNode", Guid.Empty); |
There was a problem hiding this comment.
Would still recommend wrapping this under try-catch.
There was a problem hiding this comment.
Doesn't ReadGuid() already handle failure case with Guild.Empty?
Purpose
This PR aims to address DYN-8894.
New logic to relocate groups, notes, nodes and their associated connector pins if they overlap with expanding group. This also applies when optional inports or unconnected outports are expanded.
Please note: This PR is built on top of #16302, so most of the changes included here are already present in that branch.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
New logic to relocate groups, notes, nodes and their associated connector pins if they overlap with expanding group. This also applies when optional inports or unconnected outports are expanded.
Reviewers
@DynamoDS/eidos
@jasonstratton
FYIs
@achintyabhat
@dnenov